feat: wire P2P chat bridge (send, receive, chat rooms list) - #95

rust/src/api/orders.rs (2)

992-994: Extraneous whitespace in log message.

The log message has inconsistent spacing due to string literal continuation across lines. This could make log parsing harder.

📝 Clean up log formatting

         log::warn!(
-            "[orders] on_peer_pubkey_received: session not found for order={order_id},              skipping session update — incoming subscription still spawned"
+            "[orders] on_peer_pubkey_received: session not found for order={order_id}, \
+             skipping session update — incoming subscription still spawned"
         );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rust/src/api/orders.rs` around lines 992 - 994, The log::warn! call inside
on_peer_pubkey_received contains extraneous whitespace from a broken string
literal; replace the multi-line/space-padded message with a single cleanly
formatted string (keeping the {order_id} interpolation) so it reads e.g.
"[orders] on_peer_pubkey_received: session not found for order={order_id},
skipping session update — incoming subscription still spawned" to ensure
consistent log formatting and easier parsing.

---

899-912: Verify peer pubkey extraction logic for each action type.

The logic extracts buyer_trade_pubkey for BuyerTookOrder and seller_trade_pubkey for all other cases (line 901). However, the match only covers BuyerTookOrder and HoldInvoicePaymentAccepted. For HoldInvoicePaymentAccepted, the buyer receives the seller's pubkey, which is correct. The wildcard _ on line 901 could be more explicit.

📝 Make the match exhaustive for clarity

             let peer_pubkey_hex = match kind.action {
                 Action::BuyerTookOrder => small_order.buyer_trade_pubkey.clone(),
-                _ => small_order.seller_trade_pubkey.clone(),
+                Action::HoldInvoicePaymentAccepted => small_order.seller_trade_pubkey.clone(),
+                _ => unreachable!("only BuyerTookOrder and HoldInvoicePaymentAccepted reach here"),
             };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rust/src/api/orders.rs` around lines 899 - 912, The peer pubkey selection
currently uses a wildcard for all non-BuyerTookOrder cases which is ambiguous;
update the match on kind.action so it explicitly lists the Action variants that
should use small_order.seller_trade_pubkey (e.g.,
Action::HoldInvoicePaymentAccepted and any other specific seller-side actions)
and keep Action::BuyerTookOrder mapped to small_order.buyer_trade_pubkey; use an
explicit catch-all arm only if you add a clear comment explaining why it falls
back to seller_trade_pubkey, referencing the symbols kind.action,
Action::BuyerTookOrder, Action::HoldInvoicePaymentAccepted,
small_order.buyer_trade_pubkey, and small_order.seller_trade_pubkey so the
intent is unambiguous and future additions won’t accidentally change behavior.

rust/src/api/messages.rs (2)

926-959: Test name is misleading — it documents that duplicates are not ignored.

The test name add_duplicate_message_is_ignored_in_count suggests duplicates are ignored, but the assertion assert_eq!(msgs.len(), 2) and the comments explicitly state that both messages are stored. Consider renaming to better reflect the documented behavior.

📝 Suggested test rename

-    /// Verify that the message store deduplicates by id.
-    /// subscribe_incoming_chat relies on this to ignore echo messages.
+    /// Documents that the message store does NOT deduplicate by id.
+    /// The Dart UI layer must deduplicate incoming messages.
     #[tokio::test]
-    async fn add_duplicate_message_is_ignored_in_count() {
+    async fn message_store_does_not_deduplicate_by_id() {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rust/src/api/messages.rs` around lines 926 - 959, Rename the test function
add_duplicate_message_is_ignored_in_count to reflect that the Rust store does
NOT deduplicate (e.g., add_duplicate_message_is_stored_twice or
add_duplicate_message_is_not_deduplicated_in_store) and update the test doc
comment accordingly; locate the async test function named
add_duplicate_message_is_ignored_in_count (which uses message_store(),
ChatMessage, and get_messages()) and change the function name and top comment so
they document that two messages with the same id are both persisted and the Dart
layer is responsible for deduplication.

---

654-665: Misleading comment contradicts the actual (correct) code pattern.

The comment on lines 654-655 says "Subscribe BEFORE obtaining the receiver" but the actual code correctly obtains the receiver first (line 660), then subscribes (line 662). This matches the pattern in subscribe_gift_wraps and is the correct approach to avoid missing events. The comment should be updated to match reality.

📝 Suggested comment fix

-    // Subscribe BEFORE obtaining the receiver to avoid missing events that
-    // arrive between the two calls.
+    // Obtain the receiver BEFORE subscribing to avoid missing events that
+    // arrive between the two calls — same pattern as subscribe_gift_wraps.
     let filter = nostr_sdk::Filter::new()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rust/src/api/messages.rs` around lines 654 - 665, The inline comment above
the receiver/subscription sequence is incorrect: update the comment near
client.notifications() / client.subscribe(...) in messages.rs to state that we
obtain the receiver first and then subscribe (matching the pattern used in
subscribe_gift_wraps and the actual code) so the comment reflects the correct
order and intent to avoid missing events.

lib/features/chat/providers/chat_providers.dart (1)

231-236: Consider parallelizing trade-to-room conversion for better performance.

The loop awaits each tradeInfoToChatRoom call sequentially. With many trades, this could be slow since each call may hit the Rust bridge and message store. Using Future.wait would parallelize the conversions.

⚡ Suggested parallel conversion

 final chatRoomsFromTradesProvider =
     FutureProvider<List<ChatRoomState>>((ref) async {
   final trades = await ref.watch(rawTradesProvider.future);

-  final rooms = <ChatRoomState>[];
-  for (final trade in trades) {
-    final room = await tradeInfoToChatRoom(trade);
-    if (room != null) rooms.add(room);
-  }
+  final roomFutures = trades.map(tradeInfoToChatRoom);
+  final results = await Future.wait(roomFutures);
+  final rooms = results.whereType<ChatRoomState>().toList();

   rooms.sort((a, b) => b.lastMessageAt.compareTo(a.lastMessageAt));
   return rooms;
 });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/features/chat/providers/chat_providers.dart` around lines 231 - 236, The
current sequential loop awaiting tradeInfoToChatRoom for each element (trades ->
rooms) is slow; convert it to parallel by mapping trades to a list of futures
and using Future.wait to await them concurrently, then filter out nulls to
produce a List<ChatRoomState>. Update the block that builds rooms (references:
trades, tradeInfoToChatRoom, rooms, ChatRoomState) to collect the results of
Future.wait(trades.map(...)) and then use whereType or remove nulls to get the
final rooms list.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

rust/src/api/messages.rs (1)

724-735: Verify the echo-filtering logic covers all scenarios.

The echo filter compares sender_pubkey (from the inner rumor's pubkey field) against trade_pubkey_hex. This assumes the sender always signs with their trade pubkey. If the sender ever signs with a different key (e.g., due to key rotation or an edge case), echoes could leak through.

Consider adding a comment clarifying this assumption, or verify that the protocol guarantees the inner rumor pubkey always matches the trade pubkey.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rust/src/api/messages.rs` around lines 724 - 735, The echo filter currently
compares the inner rumor's sender_pubkey to trade_pubkey_hex which assumes the
sender always signs with their trade key; update the code to either (1) document
this assumption with a clear comment near sender_pubkey/trade_pubkey_hex
explaining that protocol guarantees inner.pubkey == trade pubkey (or note if key
rotation is possible), and/or (2) strengthen the check by also comparing the
outer/top-level event pubkey (if available) or other authoritative author
identity (e.g., event.pubkey or signature-verified author) against
trade_pubkey_hex so echoes signed by a different key don't bypass the filter;
reference the sender_pubkey, inner, and trade_pubkey_hex symbols when making the
change.

lib/features/chat/screens/chat_room_screen.dart (1)

187-196: Consider using the Rust-side isRead flag instead of local recomputation.

_buildRoomPreview recomputes unreadCount from the local _messages list. Since messages arrive with isRead: false from the Rust layer and _markRead() calls the bridge to update read status, the local recomputation may drift from the bridge's state if there's any timing mismatch.

However, since _markRead() is called immediately after receiving messages and the bridge call is awaited before any UI update, this is likely fine in practice.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/features/chat/screens/chat_room_screen.dart` around lines 187 - 196,
_buildRoomPreview currently recomputes unreadCount from the local _messages list
which can drift from the Rust bridge state; instead, use the Rust-provided
unread/read state (e.g., the room model returned by _resolveRoom or the
message's isRead flag) to populate unreadCount. Replace the local recompute (the
unread variable using _messages.where(...).length) and set unreadCount to the
value provided by the resolved room (room.unreadCount) or, if the room model
lacks it, derive from the incoming rust_types.ChatMessage.isRead values provided
by the bridge, and ensure this aligns with _markRead bridge calls.

rust/src/api/orders.rs (1)

996-1001: Fix malformed log message with embedded whitespace.

The log message spans multiple lines with irregular indentation that will appear in the output. Consider consolidating to a single line or using proper formatting.

✨ Suggested fix

-        log::warn!(
-            "[orders] on_peer_pubkey_received: session not found for              order={order_id}, skipping session update —              incoming subscription still spawned"
-        );
+        log::warn!(
+            "[orders] on_peer_pubkey_received: session not found for order={order_id}, \
+             skipping session update — incoming subscription still spawned"
+        );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rust/src/api/orders.rs` around lines 996 - 1001, The log::warn! invocation
inside on_peer_pubkey_received currently contains embedded newlines and
irregular whitespace; consolidate it into a single-line message and use proper
formatting placeholders instead of inline braces so it prints cleanly, e.g.
replace the multi-line string with a single line like "session not found for
order={}, skipping session update — incoming subscription still spawned" and
pass order_id as the argument to log::warn!(...) (or use a named parameter like
order_id = order_id) to ensure correct formatting.